feat(scenarios): add firmware update success scenario - #133
Conversation
|
Thanks for this, and for writing up the reasoning in the description. The scenario Four things before I merge. The first two are on the PR, the last two are on me
I will approve the workflow run so you get CI output on the next push. One note on scope, separate from this PR. You claimed #104, #106, #107 and #108 |
…e state (#135) PR #133, the first external contribution to a good-first-issue, failed CI and needed four review items. Three of the four came from gaps in the contributor guide rather than from anything the contributor did. CONTRIBUTING.md now documents `pnpm format:check`, which CI enforces and the guide never named, with both command lists ordered to match the CI job step for step and a note that CI stops at the first failure. The hard-coded counts are out of the architecture table: the detection entry claimed 15 rules and there have been 16 since REPEATED_BOOT_NOTIFICATION landed in #114, and those counts have now drifted twice. "Adding a Scenario" names all four files carrying the scenario count and flags that only two of them fail the suite when missed. Station IDs are documented as unique per scenario. Good-first-issues carry a claim policy of one open claim at a time. Also corrects the v0.4.x release state in CURRENT_STATE.md, which reported 0.4.1 with a 0.4.2 patch in progress. 0.4.2 is published and holds the latest dist-tag, its tag and GitHub release exist, and both bugs it tracked (#127, #128) are closed. The active milestone now reads v0.5.0 with v0.4.x complete, What's Done gains the v0.4.x release record it never had, and What's Next drops the request to cut v0.4.0, which shipped on 2026-07-17. No changeset: neither file is in the package `files` list, so nothing here ships to npm. Closes #134 Closes #136
|
@sepehr-safari Thanks for the thorough review! I've pushed a fix commit addressing items 2–4:
For item 1 (Prettier formatting), I noticed the new scenario file doesn't seem to have message arrays wrapped at 80 columns in my local view — could you point me to the specific lines that need Also, thanks for clarifying the claim workflow — noted for next time. I'd like to take #104 next once this merges. |
|
Thanks, that is items 2, 3 and 4 done. I checked each against the branch: the On item 1, here is why you cannot see it. The file is not Prettier-clean under Three
Each collapses to a single line inside 100 characters. The fix is one command, pnpm formatRather than send you round a third time for one command, I ran it and pushed the For next time, the thing worth carrying over is to run Worth a look at the On what is next: #104 is this issue, the one this PR closes. I think you meant |
The changeset that came in with #133 was a `minor`, so the pending release PR proposed `0.5.0`. That number is reserved for OCPP 2.0.1 in both `ROADMAP.md` and `CURRENT_STATE.md`. Spending it on one new scenario would push the 2.0.1 work to `0.6.0` and break the milestone-to-version mapping the project has held since v0.1. Retags the pending changeset to `patch`, so the release recomputes to `0.4.3`. Nothing had published yet, so the change was free to make. Adds the convention to the "Adding a Scenario" section, which did not mention changesets at all. That omission is how the wrong size arrived: #133 was asked for a minor during review, and #137, #138 and #139 are queued behind it. A new scenario does add a public export, but scenarios are test corpus rather than consumer API surface: nothing is built against an individual scenario constant, and the registry is consumed as a whole through `scenarios` and `getScenario()`. Minor and major stay reserved for changes to the analysis engine. Closes #142
|
Merged, and it went out in #138, the firmware-update-failure scenario, is held for you. Leave a comment on It is the counterpart to the one you just wrote: same message type, the statuses the rule Two things are different from last time, both meant to save you the round trips. One correction from the guide since your PR: scenario additions take a patch If you would rather do something other than another scenario, #107 is still open |
…us work (#146) The living document drifted through two paths that bypass a normal feature PR: the release PR is created by the changesets action and never touches this file, and issue-only work leaves no PR at all. #137 through #140, #144 and the 0.4.3 release all landed without a record here. Records 0.4.3 (2026-07-28) and 0.3.2 (2026-07-14), the latter never logged at all, and corrects the package status table, which had read 0.3.1 since the v0.4.0 release. Adds a section covering the external contribution pipeline: the second good-first-issue completing, the patch-not-minor changeset decision (#142, #143), the detection-rule coverage audit that found 3 of 16 rules with no scenario, the three issues opened to close it, the retarget of #108 after its expectedFailures turned out to be unsatisfiable, and the station ID allocation across in-flight issues. It closes with the arithmetic to the v1.0 target of 20+ scenarios. Corrects an overclaim from #135, which described PR #133 as the first external contribution to a good-first-issue. It is the second: #105 carried the label and Develop-KIM completed it in #114, shipped in 0.3.1. Replaces "None currently. All design decisions resolved in ADRs" under pending decisions, since #144 carries four open questions. Closes #145
Adds a `firmware-update-failure` scenario: a station boots, reports the firmware `Downloading`, then `InstallFailed`, followed by a heartbeat. `expectedFailures` is `['FIRMWARE_UPDATE_FAILURE']`, the failing counterpart to the firmware-update-success scenario from #133. The rule treats `DownloadFailed`, `DownloadPaused`, `InstallFailed` and `InstallRebootingFailed` as failures, and this covers the `InstallFailed` path. Takes the scenario registry to 17, updating the count in the registry test, the external fixture test, and both READMEs. The detection-rule count stays 16, since a scenario is not a rule. Closes #138
…ocation (#165) Brings the narrative half of CURRENT_STATE.md forward after #161. The automated half needed nothing: #152 logged 0.4.5 and moved the Package Status Table on its own. The station ID allocation line claimed 017 was reserved for #108. It was not. refused-authorization.ts took CS-SYNTHETIC-017 when it shipped in 0.4.5, after that line was written, so anyone picking up #108 would have created the same collision #133 did. #108 now reserves 021, and the line records live allocation plus the next free number. Corrects rule coverage from two uncovered rules to one, verified by iterating the registry rather than by reading: 15 of the 16 codes appear in some scenario's expectedFailures and REPEATED_BOOT_NOTIFICATION is the only gap. Corrects the corpus arithmetic from 17 to 19. Records #161 merging as the third external contributor's first merge, both remaining scenario issues being assigned, the #108 trap caught before the contributor started, and #163 opened for the assertions-coverage gap. Closes #164
Publishing 0.4.6 left the hand-written "Current Version" paragraph at 0.4.4. The Release Log and the Package Status Table were already correct, because #152 maintains those automatically; the narrative sentence is deliberately human and has to be moved by hand. Also corrects the contribution count. With #161 shipping in 0.4.6 there are four external good-first contributions rather than three, from three different people, and the paragraph now names the three that landed in the 0.4.x line: 0.4.3 (#104, PR #133), 0.4.4 (#138, PR #147) and 0.4.6 (#137, PR #161). Closes #167
2 participants
Add a new scenario to verify that a station undergoing a successful firmware update does not trigger any false positives:
expectedFailures: []— The trace describes a clean firmware update flow (BootNotification → FirmwareStatusNotification(Dowloaded) → FirmwareStatusNotification(Installed) → Heartbeat), which should not trigger any detection rule.assertions: [{ type: 'no_failures', params: {} }]— Verifies that no failures are detected, acting as a regression guard against overly aggressive detection rules.tests/external-fixture/test.mjswas also updated (line 161:15→16) because it has a hard-coded scenario count assertion.